feat: support list blob scans#7664
Conversation
github-actions
Bot
added
A-encoding
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
# Conflicts: # rust/lance-core/src/datatypes/schema.rs
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBlob V2 handling validates descriptors more strictly, supports binary views for nested list fields, preserves descriptor nulls, and materializes binary payloads during filtered, unfiltered, and take execution paths. ChangesBlob V2 binary flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ScanPlanner
participant FragmentReader
participant BlobV2ReadContext
participant BlobMaterializer
ScanPlanner->>FragmentReader: select descriptor schema and row addresses
FragmentReader->>BlobMaterializer: provide decoded descriptor batch
BlobMaterializer->>BlobV2ReadContext: resolve descriptor payload locations
BlobV2ReadContext->>BlobMaterializer: return payload bytes
BlobMaterializer->>ScanPlanner: return public binary batch
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-core/src/datatypes/field.rs`:
- Around line 603-611: In the relevant field conversion logic, replace the bare
unwrap on LogicalType::try_from(&DataType::LargeBinary) with expect containing a
clear reason, and replace the duplicated "packed" and "lance-encoding:packed"
metadata removals with iteration over the existing PACKED_KEYS constant used by
is_packed_struct.
In `@rust/lance/src/dataset/blob.rs`:
- Around line 2338-2361: Update the blob-v2 filtered-read materialization around
materialize_blob_v2_binary_array and FilteredReadExec schema handling so
requested system columns (_rowid, _row_last_updated_at_version, and
_row_created_at_version) are preserved alongside _rowaddr. Ensure
projection.to_bare_schema() does not drop them, and that the produced columns
and output schema match FilteredReadExec::schema() for every requested system
column.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02fd15aa-ede3-45ce-859c-38adabfb62c3
📒 Files selected for processing (6)
rust/lance-core/src/datatypes/field.rsrust/lance-core/src/datatypes/schema.rsrust/lance-encoding/src/encodings/logical/blob.rsrust/lance/src/dataset/blob.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/scan.rs
| for (idx, row_addr) in row_addrs.iter().copied().enumerate() { | ||
| if descriptions.is_null(idx) || columns.kinds.is_null(idx) { | ||
| builder.append_null(); | ||
| continue; | ||
| } | ||
|
|
||
| match kind { | ||
| let kind = BlobKind::try_from(columns.kinds.value(idx))?; | ||
| if matches!(kind, BlobKind::Inline) | ||
| && columns.positions.value(idx) == 0 | ||
| && columns.sizes.value(idx) == 0 | ||
| { | ||
| builder.append_value([]); | ||
| continue; | ||
| } | ||
|
|
||
| let entry = read_context | ||
| .collect_entry(&columns, idx, idx, row_addr) | ||
| .await?; | ||
| if let Some(entry) = entry { | ||
| let data = entry.file.read().await?; | ||
| builder.append_value(data.as_ref()); |
There was a problem hiding this comment.
Blob payloads are awaited one at a time here, so a list with N remote blobs needs roughly N sequential round trips and BlobSource cannot coalesce the reads. Filtered scans also pay this cost before applying the residual filter.
| field.children = field | ||
| .children | ||
| .into_iter() | ||
| .map(|child| self.unload_if_needed(child)) |
There was a problem hiding this comment.
Filtering a blob field can still panic on this path. BlobsDescriptions replaces a blob v2 leaf with descriptor children whose IDs are all -1; the subsequent union/projection drops those children, so applying the projection selects the parent with no children and hits the assertion in Field::apply_projection. For example, blobs IS NOT NULL on the new List<Blob> path, or image.image_bytes IS NOT NULL, raises pyo3_runtime.PanicException. The filtered-read test only filters id, so this case is not covered.
2 participants
Summary
This adds scan-based support for
List<Blob>and nested blob leaves such asStruct<List<Blob>>.Descriptor scans preserve the original Arrow nesting and expose blob leaves as descriptors without loading payload bytes.
BlobHandling::AllBinarymaterializes only blob leaves to binary values while keeping the surroundingList/Structlayout intact.This keeps the existing one-blob-per-row APIs unchanged and avoids adding a public random-access API for list blob values.
Summary by CodeRabbit